Skip to content

Update dotnet sln #15694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 20, 2020
Merged

Update dotnet sln #15694

merged 9 commits into from
Feb 20, 2020

Conversation

Forgind
Copy link
Contributor

@Forgind Forgind commented Nov 5, 2019

Reordered slightly and changed wording to make dotnet sln clearer. Added an example of dotnet sln list.

Closes #15522

Change order and some wording and add an example.
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line (which you didn't change) might not be correct:

dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder] <PROJECT_PATH>

I think there should be an argument going along with --solution-folder, ie:

dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder <SOLUTION_FOLDER>] <PROJECT_PATH>

## Name

`dotnet sln` - Modifies a .NET Core solution file.
`dotnet sln` - Options for the projects in a .NET Core solution file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous wording was actually better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifies wouldn't include list. Perhaps Options for listing or modifying the projects in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should break this down into multiple pages like we did with dotnet tool

mairaw
mairaw previously requested changes Nov 7, 2019
Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Forgind. Left a couple of comments. Another option would be break the sln command page into one for each command like we have for other CLI commands, if that would be preferable as well.


#### Synopsis

```dotnetcli
dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder] <PROJECT_PATH>
dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder <SOLUTION_FOLDER>] <PROJECT_PATH> [<PROJECT_PATH>...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually show the argument in the synopsis. Instead, we add that where the option is shown.

Suggested change
dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder <SOLUTION_FOLDER>] <PROJECT_PATH> [<PROJECT_PATH>...]
dotnet sln [<SOLUTION_FILE>] add [--in-root] [-s|--solution-folder] <PROJECT_PATH> [<PROJECT_PATH>...]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw another PR where a customer was trying to add the same info. Do you two think that we should do that across the board? I closed the PR for now so we could discuss that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If switches are well-named (as in --solution-folder), I don't think it's necessary, but if -s were the only way to express that, it would be better to include <SOLUTION_FOLDER>. Since consistency is good, I would say that if there is one case in which we can't have a well-named switch, we should do it across the board. Otherwise, it's fine to take it out.

I'll take it out for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it makes sense to show the argument in the synopsis in all cases. Otherwise you can't tell the difference between a switch like --in-root which doesn't accept an argument and one like --solution-folder which requires one.

dotnet sln add [-h|--help]
```

#### Arguments

- **`SOLUTION_FILE`**

The solution file to use. If not specified, the command searches the current directory for one. If there are multiple solution files in the directory, one must be specified.
The solution file to use. The command searches the current directory for one if it's left unspecified, failing if there are multiple solution files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of our style guide to start the sentence with the condition first.


#### Options

- **`-h|--help`**

Prints out a short help for the command.
Prints out a short list of options for the command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is description for all --help options across CLI docs. If this is preferred version, we'd have to change across all docs.

dotnet sln [<SOLUTION_FILE>] remove [-h|--help]
```

#### Arguments

- **`SOLUTION_FILE`**

The solution file to use. If not specified, the command searches the current directory for one. If there are multiple solution files in the directory, one must be specified.
The solution file to use. The command searches the current directory for one if it's left unspecified, failing if there are multiple solution files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, starting with the condition is part of our style and voice

Prints out a short help for the command.

### `list`
Prints out configurable options for the command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks different from the previous description for help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included two versions to see which you like better, but I don't like the current one. It's bad style to include a word in its definition, and this usage for "help" is awkward in English in my opinion, so I would prefer to use one of my versions. Do you like either?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we change in the CLI too? It's not exactly the same but it says help too.

I don't think that --help is only about options so I think both changes are a bit misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would.

That's true. How about one of:
"Emits a description of how to use dotnet sln list"
"Prints out a description of how to use dotnet sln list"
"Describes how to use dotnet sln list"
possibly replacing "dotnet sln list" with "this command"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for "Prints out a description of how to use the command." Whatever we end up with here we can propagate to the other CLI docs later.


#### Options

- **`-h|--help`**

Prints out a short help for the command.
Prints out a short list of options for the command.

- **`--in-root`**

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 100 would be the place to add the option argument for -s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following. -s is on line 100.

@mairaw
Copy link
Contributor

mairaw commented Nov 7, 2019

Also, since we're looking at dotnet sln, I'd like to address my comment from #15522 (comment)

I don't really understand the new options for 3.0.

Also added a closes keyword at the PR description to close the related issue.

@Forgind
Copy link
Contributor Author

Forgind commented Nov 7, 2019

I don't know what the new options for 3.0 are, and when I asked @dsplaisted about what I should make sure to include that isn't already there, he said there might not be anything to update. Where did you see that there are new options for dotnet sln in 3.0?

@mairaw
Copy link
Contributor

mairaw commented Nov 10, 2019

I saw with dotnet sln add --help that there were two new options that I've already added previously:
https://review.docs.microsoft.com/en-us/dotnet/core/tools/dotnet-sln?branch=pr-en-us-15694#options-2

--in-root and -s|--solution-folder. But if all dotnet sln add is doing is adding existing projects to an existing solution, I don't really understand their intention.

@Forgind
Copy link
Contributor Author

Forgind commented Nov 12, 2019

My understanding is that when you use dotnet sln add, it also creates obj, a folder with some information related to nuget, debugging, and so on. If you use --in-root, it doesn't make that folder; if you use -s, it lets you specify where you want the folder to be created. Is that correct, @dsplaisted? Also, that explanation was based on my reading of what --help printed out, but it seemed like --in-root did the opposite—making a folder where there otherwise wouldn't be one. Is that expected behavior?

@dsplaisted
Copy link
Member

@Forgind No, dotnet sln add shouldn't create an obj folder. Building a project will typically create a bin folder for the output and an obj folder for the intermediate files, but that's not related to the solution.

A solution can have folders to organize the projects in the solution. Those folders don't necessarily need to match the folders the projects are in on disk. It sounds like dotnet sln add will by default create solution folders matching the folders on disk, but you can override the solution folders with -s, or tell it not to create any with --in-root.

@mairaw
Copy link
Contributor

mairaw commented Jan 14, 2020

Resolved merge conflicts

@mairaw mairaw requested a review from tdykstra as a code owner February 20, 2020 00:46
@mairaw
Copy link
Contributor

mairaw commented Feb 20, 2020

Resolved merge conflicts

@mairaw mairaw dismissed their stale review February 20, 2020 00:50

changes made since last review

Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a couple of suggestions for you to consider.

@@ -31,42 +31,64 @@ dotnet new sln

- **`SOLUTION_FILE`**

The solution file to use. If not specified, the command searches the current directory for one. If there are multiple solution files in the directory, one must be specified.
The solution file to use. The command searches the current directory for one if it's left unspecified, failing if there are multiple solution files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The solution file to use. The command searches the current directory for one if it's left unspecified, failing if there are multiple solution files.
The solution file to use. If the argument is omitted, the command searches the current directory. If it finds no solution file or multiple solution files, the command fails.


- **`SOLUTION_FILE`**

The solution file to use. The command searches the current directory for one if it's left unspecified, failing if there are multiple solution files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the H4 Arguments sections I would omit the description for SOLUTION_FILE since that's covered at the top in the H2 Arguments section. If we keep the H4 descriptions we should make sure they're all the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep them, since I can easily imagine people skipping down to what they think is relevant, then getting confused. I made this one match the one above, though.

Prints out a short help for the command.

### `list`
Prints out configurable options for the command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for "Prints out a description of how to use the command." Whatever we end up with here we can propagate to the other CLI docs later.

@tdykstra tdykstra merged commit 811b1e2 into master Feb 20, 2020
@tdykstra tdykstra deleted the dotnet-sln-docs-update branch February 20, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review dotnet sln documentation
6 participants